You don't need to copy the request here - you can std::move the request and create the copy inside the scheduler callback.

I am not sure about that. The request is const arg so we should not std::move it

std::move-ing doesn't guarantee that a move will occur, it only tells the compiler it may move it.

yes and accessing the old reference is undefined behavior. Hence I am hesitant to do what you suggest

We're not accessing the old reference though, so I'm not sure I see the problem. If you do prefer to eagerly copy here, then we don't need to copy it again on line 45.

My point is a const arg should not be modified. And by moving it we will invalidate it.

on line 45 we do need to copy it again since it's not modifiable since its an rvalue.

pardon me, on line 45 its not a copy. It's a move so as efficient as it can be.

You can just make the argument not be const. Top-level const on arguments is ignored when deciding the function's type, so void(foo) and void(const foo) have the same type and can both be assigned to a void(*)(foo) function pointer. The const is only meaningful inside the implementation of the function as it makes the parameter value behave like a const object. In this case, that is probably something you don't want.

std::move-ing doesn't guarantee that a move will occur, it only tells the compiler it may move it.

Sort of. std::move makes its result a (possibly const) rvalue. That then can affect overload resolution specifically to choose overloads that then move from the object. All of this is specifically defined, and no part of it is up to the compiler (except for the poorly named copy-elision optimization where it is allowed to completely skip both copies and moves in some cases).

yes and accessing the old reference is undefined behavior.

Not quite, although it is perhaps a good rule of thumb to pretend this is the case most of the time. A) it isn't the std::move that causes this affect, it is whether a move actually occurs. B) For all standard library types, you are guaranteed that a moved-from object will be left in a "valid but unspecified state". This means that you can call any methods that don't have preconditions. For user-defined types, unless otherwise stated, it is best to assume that you can only destroy the object or assign over it. "Named assignments" such as clear() or reset() are also generally considered acceptable.

(As I said, only using std::move on object you don't touch again is a good safe rule of thumb if you don't want to think too hard. But this thread was already getting into the weeds, so I wanted to make sure there was a precise statement of what the real rules are)

Thanks @RedBeard0531. Good insights. I was with the impression that const args will help the compiler in someway optimize the function invocation. I will remove the const from the arg there.

Why is this copy necessary?

on line 45 we do need to copy it again since it's not modifiable since its an rvalue.

In other words you get this error if you try to modify request_copy.url
error C3490: 'url' cannot be modified because it is being accessed through a const object

Btw this realm_http_request_t request = std::move(request_copy); is not a copy

It's a move so as efficient as it can be.

Also, if you add mutable to the lambda, you can get rid of the realm_copy variable.

BTW, if you are OK writing this part in C++, why not just implement the C++ GenericNetworkTransport API directly?

Will fix

Btw the C-API definition of the http request is this one

 /**
 * Callback function used by Core to make a HTTP request.
 *
 * Complete the request by calling realm_http_transport_complete_request(),
 * passing in the request_context pointer here and the received response.
 * Network request are expected to be asynchronous and can be completed on any thread.
 *
 * @param request The request to send.
 * @param request_context Internal state pointer of Core, needed by realm_http_transport_complete_request().
 */
typedef void (*realm_http_request_func_t)(realm_userdata_t userdata, const realm_http_request_t request,
                                          void* request_context);

Shouldn't that be a const realm_http_request_t& request instead. Why do they insist on copying the request object while invoking the function.

This is how they invoke it https://github.com/realm/realm-core/blob/master/src/realm/object-store/c_api/http.cpp#L60-L63.

So why not pass the second argument by reference?

@RedBeard0531 I did the fix. Could you please, check the PR again. Also this message above :)

ok I spoke with Yavor about this. seems no point on doing that. there will be not much gain

You can just make the argument not be const. Top-level const on arguments is ignored when deciding the function's type, so void(foo) and void(const foo) have the same type and can both be assigned to a void(*)(foo) function pointer. The const is only meaningful inside the implementation of the function as it makes the parameter value behave like a const object. In this case, that is probably something you don't want.

std::move-ing doesn't guarantee that a move will occur, it only tells the compiler it may move it.

Sort of. std::move makes its result a (possibly const) rvalue. That then can affect overload resolution specifically to choose overloads that then move from the object. All of this is specifically defined, and no part of it is up to the compiler (except for the poorly named copy-elision optimization where it is allowed to completely skip both copies and moves in some cases).

yes and accessing the old reference is undefined behavior.

Not quite, although it is perhaps a good rule of thumb to pretend this is the case most of the time. A) it isn't the std::move that causes this affect, it is whether a move actually occurs. B) For all standard library types, you are guaranteed that a moved-from object will be left in a "valid but unspecified state". This means that you can call any methods that don't have preconditions. For user-defined types, unless otherwise stated, it is best to assume that you can only destroy the object or assign over it. "Named assignments" such as clear() or reset() are also generally considered acceptable.

(As I said, only using std::move on object you don't touch again is a good safe rule of thumb if you don't want to think too hard. But this thread was already getting into the weeds, so I wanted to make sure there was a precise statement of what the real rules are)

The real answer here is that you want to add mutable to the lambda. Without it, all by-value captures, including request_copy are considered as const inside the body of the lambda. So request is silently making a copy, even though there is a std::move. You probably should also remove the const from the original request parameter, and the request_copy variable and just capture request as ~request = std::move(request)~~ simply request (see next paragraph). And if it the lambda is marked mutable, you can just modify it in place without making another copy. Since the callback will only be called once this is fine.

Oh. I just noticed that realm_http_request_t is the C API type. For C types (or any other "Trivially Copiable" type in C++) there is no difference between move and copy. So all of this discussion of move vs copy is moot, and you might as well just copy since it is syntactically lighter. I'm leaving what I've already written about std::move in the hope that it is informative.

std::move is not a copy its more of a const_cast

To be pedantic, const_cast can only add or remove const and volatile qualifiers. std::move is specifically a static_cast that adds turns an lvalue into an rvalue. It does NOT remove the const. But it also doesn't copy (although the thing you pass the result to may make a copy of the referenced object).

my understanding is that you cannot std::move a const object.

You can std::move a const object, but you probably shouldn't. It produces a const T&& which is a bit of a weird thing. While a type could have a special constructor just for that, in general it will select the copy constructor during overload resolution and make a copy. So the move isn't harmful (in that it doesn't change the behavior of the program, although it may make more template instantiations and potentially slow down compiles or bloat code size), but it is misleading. We could consider making our own realm::move that will cause a compiler error on const objects so that you know to either remove the const or remove the no-op move. The reason std::move allows passing const object is both for consistency, and because it is useful in templates where there are some cases where you want to move from non-const objects but obviously you can't from const ones, but you don't know what you will be passed and want to work with either. But that isn't something we do often.

As far as I understand, std::move doesn't really do anything for that struct.

That is true for buf as well as I understand it. I kept it for consistency. Otherwise whoever reads it later will wonder why we are not moving request but we are moving buf. Also for c++ I prefer being explicit.

If it doesn't do anything, then let's remove it from buf as well - no point in pretending we move something.

Why do you think std::move(buf) doesn't do anything? buf is not a trivially copiable C-like type. Note that it's members include C++ lifetime aware types. This id different from request which is a plain C-like struct, and is required to be as part of the C API.

TL;DR: I agree with the suggested change.

Edit: that said, after looking at this, it may be even more efficient to do auto buf = std::make_unique<request_copy_buf>, and possibly put all of your captures in the buf. It will be an extra allocation, but then you will have less moves to do. Right now, I think you are moving each member at least twice, and that adds up, maybe to more than a malloc/free pair. Your choice if you want to do this, or avoid the allocation.

I prefer to leave that so it is more understandable what the intention is. Otherwise you really need to understand there is no difference in this particular case.

Fair enough. I am not maintaining this code, so if the people who will be think this will be more maintainable, I won't stand in your way.

Why do you think std::move(buf) doesn't do anything? buf is not a trivially copiable C-like type. Note that it's members include C++ lifetime aware types. This id different from request which is a plain C-like struct, and is required to be as part of the C API.

TL;DR: I agree with the suggested change.

Edit: that said, after looking at this, it may be even more efficient to do auto buf = std::make_unique<request_copy_buf>, and possibly put all of your captures in the buf. It will be an extra allocation, but then you will have less moves to do. Right now, I think you are moving each member at least twice, and that adds up, maybe to more than a malloc/free pair. Your choice if you want to do this, or avoid the allocation.

I think the std::move of error is meaningless here as well. In fact all comments carry over.

I am amused that this code is basically identical to the above, but unfortunately, due to the structure of the c api, it is just different enough that I don't think there is a clean way to avoid the code duplication.

FYI, it is probably better to use a std::deque<std::pair<std::string, std::string>> here. You could also use a std::vector<...> as long as you reserve() the correct size up front, or do two passes, one to push everything, and another to push collect the addresses of the strings. You just need to avoid a growth event after collecting the addresses.

Please add a comment stating that you are only updating the addresses due to moves, and that all other fields of the request, such as sizes do not need to be modified.

Alternatively, since you know the sizes of everything, You might as well just recreate the whole request from scratch rather than moving it around. Your call though.